Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docs: Add warning about snapshot_ids arg in expired_snapshots procedure #12291

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

estherbester
Copy link

@estherbester estherbester commented Feb 16, 2025

@github-actions github-actions bot added the docs label Feb 16, 2025
@@ -270,7 +270,8 @@ the `expire_snapshots` procedure will never remove files which are still require
| `retain_last` | | int | Number of ancestor snapshots to preserve regardless of `older_than` (defaults to 1) |
| `max_concurrent_deletes` | | int | Size of the thread pool used for delete file actions (by default, no thread pool is used) |
| `stream_results` | | boolean | When true, deletion files will be sent to Spark driver by RDD partition (by default, all the files will be sent to Spark driver). This option is recommended to set to `true` to prevent Spark driver OOM from large file size |
| `snapshot_ids` | | array of long | Array of snapshot IDs to expire. |
| `snapshot_ids` | | array of long | Array of snapshot IDs to expire (note that the table's expiration properties will still be applied to remove all expired snapshots, unless `older_than` or `retain_last` arguments are also given). |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this note is clearer. I'm thinking about words like Additional array of snapshots IDs to expire, besides those expired by older_thanandretain_last arguments, and table properties.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick feedback! Reworded.

@kevinjqliu
Copy link
Contributor

I lost some context here, I remember there was a devlist discussion around this. could you link that to the PR?

@kevinjqliu
Copy link
Contributor

Would be helpful to add the specific code path for snapshot_ids in expired_snapshots
For example,

if (snapshotIds != null) {
for (long snapshotId : snapshotIds) {
action.expireSnapshotId(snapshotId);
}
}

for (long id : expiredSnapshotIds) {
expireSnapshots = expireSnapshots.expireSnapshotId(id);
}

@estherbester
Copy link
Author

@kevinjqliu thanks for the feedback! I thought it might be more useful to link to the full class to show in context why the argument works the way it currently does. Also wasn't sure where exactly the links would go -- I didn't want to introduce too much inconsistency with the rest of the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants